feat(storage): add feature flag and models for appendable upload#5631
feat(storage): add feature flag and models for appendable upload#5631vsharonlynn wants to merge 3 commits into
Conversation
product-auto-label
Bot
added
the
api: storage
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the initial infrastructure for appendable object uploads in the storage crate, gated by a new unstable configuration flag. The changes include updates to build triggers, lint configurations, and the addition of new data structures and modules. Feedback focuses on maintaining SDK consistency by using ergonomic model types instead of raw proto types, applying #[allow(dead_code)] to structs used only in mocks to prevent compiler warnings, and restricting module visibility to pub(crate) to align with existing internal modules.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5631 +/- ##
==========================================
- Coverage 97.96% 97.96% -0.01%
==========================================
Files 221 221
Lines 51494 51536 +42
==========================================
+ Hits 50447 50488 +41
- Misses 1047 1048 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
vsharonlynn
force-pushed
the
feat/storage-appendable-upload
branch
from
144a189 to
3deeb71
Compare
Introduces the `google_cloud_unstable_storage_appendable_upload` feature flag to gate the upcoming GCS Appendable Uploads functionality.
vsharonlynn
force-pushed
the
feat/storage-appendable-upload
branch
3 times, most recently
from
30b9d9a to
93a0318
Compare
vsharonlynn
force-pushed
the
feat/storage-appendable-upload
branch
from
93a0318 to
790d68f
Compare
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
This points to a very low-level abstraction for appendable objects... you are defining public APIs that are direct mappings of the proto messages.
Ultimately, this is your call, but I think you want to raise the level of abstraction. Maybe think about what logical operations the application would want to do (I think "open an object for exclusive appends" and "append to an open object", and maybe "reopen an object for appends"). Implement those abstractions and hide the details from your customers.
FYI: @joshuatants
| gcb_secret_name = "projects/${var.project}/secrets/github-github-oauthtoken-319d75/versions/latest" | ||
|
|
||
| unstable_flags = join(" ", [ | ||
| "--cfg google_cloud_unstable_storage_appendable_upload", |
There was a problem hiding this comment.
You can use unstable_storage_bidi, it is not being used for anything else.
| pub enum AppendObjectFirstMessage { | ||
| /// Create a new appendable object. | ||
| Create(Box<crate::model::WriteObjectSpec>), | ||
| /// Take over an existing appendable object. | ||
| Takeover(AppendObjectSpec), | ||
| } |
There was a problem hiding this comment.
This seems like it should be two separate methods, not one method with an enum type.
Thanks for your input Carlos! Sounds like a good idea. We're actively looking into this. I'm currently drafting the APIs to see how it looks. I'll get back soon. |
vsharonlynn
force-pushed
the
feat/storage-appendable-upload
branch
from
14655b3 to
2d91381
Compare
Use existing `google_cloud_unstable_storage_bidi`, because currently it's not being used for anything. Separate the API into two, instead of one.
vsharonlynn
force-pushed
the
feat/storage-appendable-upload
branch
from
2d91381 to
45c1729
Compare
coryan
left a comment
coryan
left a comment
There was a problem hiding this comment.
Consider the nits and then merge.
| #[non_exhaustive] | ||
| pub struct OpenAppendableObjectRequest { | ||
| /// The object attributes and pre-conditions for the open operation. | ||
| pub spec: Box<crate::model::WriteObjectSpec>, |
There was a problem hiding this comment.
Unclear why you are using Box<T> here, but that is not enough reason to block the change.
| #[cfg(google_cloud_unstable_storage_bidi)] | ||
| /// Represents the parameters of a request to open a new object for exclusive appends. | ||
| /// | ||
| /// This type might be used in mocks of the `Storage` client. |
There was a problem hiding this comment.
nit:
| /// This type might be used in mocks of the `Storage` client. | |
| /// Consumers of the `google-cloud-storage` crate rarely have a need to use this type directly, the most common exception is when mocking of the `Storage` client. |
| #[cfg(google_cloud_unstable_storage_bidi)] | ||
| /// Represents the parameters of a request to reopen an existing object for appends. | ||
| /// | ||
| /// This type might be used in mocks of the `Storage` client. |
There was a problem hiding this comment.
same nit:
| /// This type might be used in mocks of the `Storage` client. | |
| /// Consumers of the `google-cloud-storage` crate rarely have a need to use this type directly, the most common exception is when mocking of the `Storage` client. |
2 participants
Introduces the
google_cloud_unstable_storage_appendable_uploadfeature flag to gate the upcoming GCS Appendable Upload functionality.